-
Notifications
You must be signed in to change notification settings - Fork 298
Cosmos: Fix #2824 by forcing 'enable_content_response_on_write' on for read item #2894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes Cosmos Database issue #2824 by ensuring read operations return content, and addresses test failures after HTTP behavior changes in #2834. The fix prevents read operations from returning empty responses when the enable_content_response_on_write
flag is disabled.
Key changes:
- Added error handling for non-2xx HTTP status codes in the Cosmos pipeline
- Force
enable_content_response_on_write
totrue
for read item operations - Updated test code to properly handle HTTP error responses
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
sdk/typespec/typespec_client_core/src/http/response.rs | Added success() method to validate HTTP status codes and convert errors |
sdk/cosmos/azure_data_cosmos/src/pipeline/mod.rs | Implemented error handling for non-2xx responses in pipeline operations |
sdk/cosmos/azure_data_cosmos/src/clients/container_client.rs | Fixed read_item to force content response flag to true |
sdk/cosmos/azure_data_cosmos/tests/framework/test_data/mod.rs | Updated test to use new error handling pattern |
sdk/cosmos/azure_data_cosmos/tests/framework/test_account.rs | Updated test to use new error handling pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few thoughts, but nothing blocking. Creative!
} | ||
} | ||
|
||
impl ResponseExt for RawResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chlowell this is a good idea we might consider in azure_core
. Combined with #2725 we could also use something like this to wrap standard Azure errors. We'd have to implement it (foreign trait otherwise), but could also allow clients to pass in custom success codes or something, much like .NET does for their DPG clients and HTTP status code categorizations.
3691354
to
6fb7837
Compare
/azp run rust - pullrequest |
Azure Pipelines successfully started running 1 pipeline(s). |
This PR has two changes in one, but I think it's reasonable:
Ok
. We now convert non-2xx status codes to errors in the Cosmos Pipelineenable_content_response_on_write
flag totrue
when reading an item.I needed to do 1 in order for the tests to properly start failing on errors after #2834, and then I went in to fix the bug. Otherwise a pretty straightforward change :)